Repository navigation
ENH - GLOWS: Reduce scope of integration tests - #235
Conversation
leowerneck
added
enhancement
There was a problem hiding this comment.
🟢 Approval recommended
The focused test-only changes consistently reduce processing scope without affecting production behavior.
0 open findings
What changed in this PR
Reduces GLOWS integration-test runtime by limiting L3e processing to selected repointings.
Changes:
- Adds an L3e repointing filter for integration tests.
- Aligns production-test versions and supports safe reruns.
- Narrows force-reprocessing fixture data to repointing 36.
| File | Description |
|---|---|
tests/integration/test_glows_processor_integration.py |
Limits L3e scope and improves Docker test reruns. |
tests/test_data/glows/imap_glows_force-reprocessing-config_20250101_v000.csv |
Restricts forced processing to repointing 36. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jtniehof
left a comment
jtniehof
left a comment
There was a problem hiding this comment.
Quick look and some quick questions, no deep dive just yet :)
| return patch( | ||
| "imap_l3_processing.glows.l3e.glows_l3e_initializer.identify_versions_for_l3e_output_files", | ||
| side_effect=identify_subset, | ||
| ) |
There was a problem hiding this comment.
The implication I'm drawing is that this is now no longer quite as much of an end-to-end integration test because we're monkey-patching identify_versions_for_l3e_output_files and losing that link of the chain. Am I being a bit paranoid here? Do we separately check the expected set of repointings either here or in a separate unit test?
There was a problem hiding this comment.
That's a good point. I changed it to return the unfiltered output back. Let me know if you agree with the changes.
| l3e_survival-probability-ul-hf repoint36, repoint37, repoint38, repoint39, repoint40, repoint41, repoint42, repoint43, repoint44, repoint45, repoint46, repoint47, repoint48, repoint49, repoint50, repoint51, repoint52, repoint53, repoint54, repoint55, repoint56, repoint57, repoint58, repoint59, repoint60, repoint61, repoint62, repoint63, repoint64, repoint65, repoint66, repoint67, repoint68, repoint69, repoint70, repoint71, repoint72, repoint73, repoint74, repoint75, repoint76, repoint77, repoint78, repoint79, repoint80, repoint81, repoint82, repoint83, repoint84, repoint85, repoint86, repoint87, repoint88, repoint89, repoint90, repoint91, repoint92, repoint93, repoint94, repoint95, repoint96, repoint97, repoint98, repoint99, repoint100, repoint101, repoint102, repoint103, repoint104, repoint105, repoint106, repoint107, repoint108, repoint109, repoint110, repoint111, repoint112, repoint113, repoint114, repoint115, repoint116, repoint117, repoint118, repoint119, repoint120, repoint121, repoint122, repoint123, repoint124, repoint125, repoint126, repoint127, repoint128, repoint129, repoint130, repoint131, repoint132, repoint133, repoint134, repoint135, repoint136, repoint137, repoint138, repoint139, repoint140, repoint141, repoint142, repoint143, repoint144, repoint145, repoint146, repoint147, repoint148, repoint149, repoint150, repoint151, repoint152, repoint153, repoint154, repoint155, repoint156, repoint157, repoint158, repoint159, repoint160, repoint161 | ||
| l3e_survival-probability-ul-sf repoint36, repoint37, repoint38, repoint39, repoint40, repoint41, repoint42, repoint43, repoint44, repoint45, repoint46, repoint47, repoint48, repoint49, repoint50, repoint51, repoint52, repoint53, repoint54, repoint55, repoint56, repoint57, repoint58, repoint59, repoint60, repoint61, repoint62, repoint63, repoint64, repoint65, repoint66, repoint67, repoint68, repoint69, repoint70, repoint71, repoint72, repoint73, repoint74, repoint75, repoint76, repoint77, repoint78, repoint79, repoint80, repoint81, repoint82, repoint83, repoint84, repoint85, repoint86, repoint87, repoint88, repoint89, repoint90, repoint91, repoint92, repoint93, repoint94, repoint95, repoint96, repoint97, repoint98, repoint99, repoint100, repoint101, repoint102, repoint103, repoint104, repoint105, repoint106, repoint107, repoint108, repoint109, repoint110, repoint111, repoint112, repoint113, repoint114, repoint115, repoint116, repoint117, repoint118, repoint119, repoint120, repoint121, repoint122, repoint123, repoint124, repoint125, repoint126, repoint127, repoint128, repoint129, repoint130, repoint131, repoint132, repoint133, repoint134, repoint135, repoint136, repoint137, repoint138, repoint139, repoint140, repoint141, repoint142, repoint143, repoint144, repoint145, repoint146, repoint147, repoint148, repoint149, repoint150, repoint151, repoint152, repoint153, repoint154, repoint155, repoint156, repoint157, repoint158, repoint159, repoint160, repoint161 No newline at end of file | ||
| ignore-after: 2100-01-01 00:00:00+00:00 | ||
| l3e_survival-probability-hi-45 repoint36 |
There was a problem hiding this comment.
Based on the CS principle that the only numbers are zero, one, and many, does it make sense to include two repoints for at least one of the outputs?
There was a problem hiding this comment.
Fair. I added repoint20 to the CSV, while the Python code calls for 36 and 49. This way we can test whether the CSV is having any impact at all [removing repoint20 from the CSV and running causes the test to fail, so the answer is yes :)].
46f0f3e to
936c4a8
Compare
jtniehof
left a comment
jtniehof
left a comment
There was a problem hiding this comment.
Excellent, thanks! Let's do this.
c872a50
into
IMAP-Science-Operations-Center:main
Change Summary
Closes #233.
Overview
Reduce scope of GLOWS integration tests to a small subset of repointings instead of all of them.
A sanity check from @jtniehof will be appreciated. The "get a subset of repointings" was the easiest choice, but maybe this was a poor choice for reasons I'm missing.
File changes
tests/integration/test_glows_processor_integration.pyonly_process_l3e_for()helper to select a subset of repointings.test_run_glows_l3be_against_prodnow uses major version 2, matching prod.test_run_glows_l3be_against_prodoverwrites output, preventing "file already exists" errors on consecutive runs.-e IMAP_API_KEY.tests/test_data/glows/imap_glows_force-reprocessing-config_20250101_v000.csvignore-afterdate in the far future and list only repointing 36 for L3e descriptor.Testing
Running:
results in tests passing in <10 minutes (reduced from >100 minutes).
Related Issues
Will note that further performance considerations might be needed when fixing #234. I will wait to address #234 until this is merged so I know the approach is sensible.